Improve detection of the type from a PHP variable #2814

GromNaN · 2025-09-01T16:24:25Z

Q	A
Type	feature
BC Break	no
Fixed issues	Related to #2789

Summary

The method getTypeFromPHPVariable is used only by convertPHPToDatabaseValue. Which is used to process expressions in the aggregation builder.

With this new feature, we try to find the type for object class name; assuming custom types are registered using the FQCN of the objects they store.

docs/en/reference/custom-mapping-types.rst

jmikola · 2025-09-02T18:05:38Z

docs/en/reference/custom-mapping-types.rst

+
+This pattern can be adapted for any custom class—just implement the conversion logic for your value object.
+
+.. |FQCN| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>


@GromNaN: this looks like it may be a false positive, since this is some very niche RST syntax

docs/en/reference/custom-mapping-types.rst

lib/Doctrine/ODM/MongoDB/Types/InvalidTypeException.php

src/Types/Type.php

lib/Doctrine/ODM/MongoDB/Types/Type.php

GromNaN

Thanks for the review. I added the type detection for DTO class name in property type.

docs/en/reference/custom-mapping-types.rst

GromNaN · 2025-09-03T12:57:19Z

docs/en/reference/custom-mapping-types.rst

+
+This pattern can be adapted for any custom class—just implement the conversion logic for your value object.
+
+.. |FQCN| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>


I revert changes on this line. I'm not sure how this will be formatted.

docs/en/reference/custom-mapping-types.rst

lib/Doctrine/ODM/MongoDB/Types/Type.php

GromNaN · 2025-09-03T16:02:46Z

docs/en/reference/custom-mapping-types.rst

+By using the |FQCN| of the value object class as the type name, the type is
+automatically used when encountering a property of that class. This means you
+can omit the ``type`` option when defining the field mapping::
+
+.. code-block:: php
+
+    #[Field]
+    public Ramsey\Uuid\Uuid $id;


This is a new feature that can compete with embedded documents: adding custom logic to convert a value object into BSON.

Maybe the example should be with a custom class instead, since UUID are new supported using symfony/uid #2826

GromNaN · 2025-09-03T16:27:30Z

src/Types/InvalidTypeException.php

We don't have a global namespace for exception like it is generally done in libraries. Existing exception names are:

Doctrine\ODM\MongoDB\MongoDBException

Doctrine\ODM\MongoDB\ConfigurationException

Doctrine\ODM\MongoDB\DocumentNotFoundException

Doctrine\ODM\MongoDB\Mapping\MappingException

Doctrine\ODM\MongoDB\Hydrator\HydratorException

Doctrine\ODM\MongoDB\LockException

We could introduce a marker interface similar to how we have MongoDB\Driver\Exception\Exception in the extension.

Copilot

Pull Request Overview

This PR improves type detection for PHP variables in the MongoDB ODM by enhancing the getTypeFromPHPVariable method to automatically detect custom types based on object class names. The change allows custom types registered using their FQCN to be automatically selected when processing aggregation expressions.

Key changes:

Enhanced type detection to check if an object's class name corresponds to a registered custom type
Improved exception handling by introducing a dedicated InvalidTypeException
Added automatic type inference for object properties when custom types are registered with class FQCNs

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`lib/Doctrine/ODM/MongoDB/Types/Type.php`	Enhanced `getTypeFromPHPVariable` method to detect custom types by class name and improved type detection logic
`lib/Doctrine/ODM/MongoDB/Types/InvalidTypeException.php`	New dedicated exception class for invalid type scenarios
`lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php`	Added automatic type detection for properties with registered custom types
`tests/Doctrine/ODM/MongoDB/Tests/Types/TypeTest.php`	Added comprehensive test coverage for the new type detection functionality
`tests/Doctrine/ODM/MongoDB/Tests/Functional/CustomTypeTest.php`	Added tests demonstrating custom type detection and proper test setup/teardown
`tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2789Test.php`	Improved test isolation by properly managing type registry state
`docs/en/reference/custom-mapping-types.rst`	Updated documentation with UUID custom type example and FQCN usage patterns

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tests/Tests/Types/TypeTest.php

tests/Tests/Functional/CustomTypeTest.php

src/Types/Type.php

docs/en/reference/custom-mapping-types.rst

jmikola · 2025-09-11T15:09:07Z

docs/en/reference/custom-mapping-types.rst

+                return $value;
+            }
+
+            if ($value instanceof Binary) {


Would it make sense to restrict this to a MongoDB\BSON\Binary where getType() returns either TYPE_OLD_UUID or TYPE_UUID? As-is, it's not clear what would happen if you encountered binary data from the server with a different sub-type.

I'll note that convertToDatabaseValue() yields a Binary object with TYPE_UUID, so perhaps there's no reason to accept TYPE_OLD_UUID here.

If you are intentionally being liberal in what you accept and conservative in what you emit (Robustness Principle) and would like to leave this as-is, that's fine by me. You can optionally add a comment here explaining that design decision for the benefit of readers.

jmikola · 2025-09-11T15:12:48Z

docs/en/reference/custom-mapping-types.rst

+                return null;
+            }
+
+            if ($value instanceof Binary) {


Would you want to ensure you always yield a Binary with sub-type TYPE_UUID? If so, you might need to split this into two separate conditionals.

if ($value instanceof Binary && $value->getType() !== Binary::TYPE_UUID) { // Note: this may throw if the data is not exactly 16 bytes return new Binary($value->getBytes(), Binary::TYPE_UUID); } if ($value instanceof Binary) { return $value; }

I'd rather simplify the example.
https://github.com/doctrine/mongodb-odm/pull/2814/files#r2393846289

src/Mapping/ClassMetadata.php

docs/en/reference/custom-mapping-types.rst

GromNaN · 2025-10-01T08:35:19Z

docs/en/reference/custom-mapping-types.rst

+            if (null === $value || [] === $value) {
+                return null;
+            }
+
+            if ($value instanceof Binary) {
+                return $value;
+            }


I can simplify the example and remove this edge cases.

Suggested change

if (null === $value || [] === $value) {

return null;

}

if ($value instanceof Binary) {

return $value;

}

if (null === $value) {

return null;

}

Without the $value instanceof Binary check, wouldn't the method then throw InvalidArgumentException for a Binary value? Or are you comfortable assuming that a Binary would never be passed in?

I didn't follow why [] was originally being handled alongside null, so I don't have much thought on that change.

In the example of document class, the property has the ?\Ramsey\Uuid\Uuid type, which ensure the RamseyUuidType::convertToDatabaseValue() method never get anything else.
As soon as the field value is the database is not corrupted, the simple implementation will work perfectly. No need to add support for edge cases that are never encountered.

GromNaN · 2025-10-01T08:35:56Z

docs/en/reference/custom-mapping-types.rst

+                return null;
+            }
+
+            if ($value instanceof Binary) {


I'd rather simplify the example.
https://github.com/doctrine/mongodb-odm/pull/2814/files#r2393846289

GromNaN · 2025-10-01T08:36:53Z

docs/en/reference/custom-mapping-types.rst

+By using the |FQCN| of the value object class as the type name, the type is
+automatically used when encountering a property of that class. This means you
+can omit the ``type`` option when defining the field mapping::
+
+.. code-block:: php
+
+    #[Field]
+    public Ramsey\Uuid\Uuid $id;


Maybe the example should be with a custom class instead, since UUID are new supported using symfony/uid #2826

jmikola

I'll defer to @alcaeus to sign off on this.

docs/en/reference/custom-mapping-types.rst

src/Types/InvalidTypeException.php

GromNaN · 2025-10-10T15:29:55Z

src/Types/Type.php

-            if ($variable instanceof ObjectId) {
-                return self::getType(self::ID);


I must check why this could be removed.

In fact, this type detection is invalid when the property type is ObjectId:

the IdType class does nothing as the target type is also ObjectId

the IdType class convert the ObjectId received from the Database into string, which is invalid when assigning the value to the ObjectId property.

alcaeus

Changes LGTM, but I would prefer changing out the UUID example.

alcaeus · 2025-10-24T08:52:58Z

docs/en/reference/custom-mapping-types.rst


        <field field-name="field" type="date_with_timezone" />

+Custom Type Example: Mapping a UUID Class


With UUID merged, WDYT of using moneyphp as an example?

I updated the tutorial.

alcaeus · 2025-10-24T08:57:32Z

src/Mapping/ClassMetadata.php

+            return $mapping;
+        }
+
+        if (! $type->isBuiltin() && Type::hasType($type->getName())) {


If I'm not mistaken, this would allow us to use a FQCN as identifier for a type name to automatically link values of that class with a given type? I haven't even thought of that before, but it makes so much sense!

alcaeus · 2025-10-24T08:58:13Z

src/Types/InvalidTypeException.php

We could introduce a marker interface similar to how we have MongoDB\Driver\Exception\Exception in the extension.

alcaeus

LGTM with two suggestions for the docs

alcaeus · 2025-10-24T13:48:07Z

docs/en/reference/custom-mapping-types.rst

+.. code-block:: php
+
+    #[Field(type: \Money\Money::class)]
+    public ?\Money\Money $price;
+
+By using the |FQCN| of the value object class as the type name, the type is
+automatically used when encountering a property of that class. This means you
+can omit the ``type`` option when defining the field mapping::


I would remove the first example and explain that the mapping logic automatically detects the property type and uses it as field type.

docs/en/reference/custom-mapping-types.rst

GromNaN added the Feature label Sep 1, 2025

GromNaN commented Sep 1, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

GromNaN requested a review from Copilot September 1, 2025 16:26

This comment was marked as outdated.

Sign in to view

GromNaN mentioned this pull request Sep 1, 2025

Unable to use #[Version] with custom type #2789

Closed

GromNaN requested review from jmikola and paulinevos September 2, 2025 14:13

paulinevos reviewed Sep 2, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

jmikola reviewed Sep 2, 2025

View reviewed changes

GromNaN force-pushed the type-from-php-variable branch from d446495 to d106a5a Compare September 3, 2025 15:59

GromNaN commented Sep 3, 2025

View reviewed changes

GromNaN requested review from Copilot, jmikola and paulinevos September 3, 2025 16:51

Copilot AI reviewed Sep 3, 2025

View reviewed changes

tests/Tests/Types/TypeTest.php Show resolved Hide resolved

tests/Tests/Functional/CustomTypeTest.php Show resolved Hide resolved

src/Types/Type.php Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Show resolved Hide resolved

GromNaN commented Sep 10, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

jmikola reviewed Sep 11, 2025

View reviewed changes

alcaeus reviewed Sep 15, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

GromNaN added this to the 2.13.0 milestone Sep 24, 2025

GromNaN commented Oct 1, 2025

View reviewed changes

GromNaN force-pushed the type-from-php-variable branch 4 times, most recently from 840f7b8 to f0ecc10 Compare October 3, 2025 14:31

jmikola reviewed Oct 10, 2025

View reviewed changes

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

docs/en/reference/custom-mapping-types.rst Outdated Show resolved Hide resolved

GromNaN commented Oct 10, 2025

View reviewed changes

src/Types/InvalidTypeException.php Show resolved Hide resolved

GromNaN commented Oct 10, 2025

View reviewed changes

GromNaN removed this from the 2.13.0 milestone Oct 17, 2025

alcaeus reviewed Oct 24, 2025

View reviewed changes

GromNaN added 2 commits October 24, 2025 15:08

Improve detection of the type from a PHP variable

a50217c

Simplify type class code to the essential

6e5d951

GromNaN force-pushed the type-from-php-variable branch from 61cf72e to f751500 Compare October 24, 2025 13:25

GromNaN changed the base branch from 2.13.x to 2.14.x October 24, 2025 13:25

GromNaN force-pushed the type-from-php-variable branch 2 times, most recently from 9857b15 to a90d2ba Compare October 24, 2025 13:31

alcaeus approved these changes Oct 24, 2025

View reviewed changes

Use MoneyPHP in the tutorial

6cd2006

GromNaN force-pushed the type-from-php-variable branch from 3a7b1bf to 6cd2006 Compare October 24, 2025 14:14

GromNaN merged commit 7dda0b7 into doctrine:2.14.x Oct 24, 2025
25 checks passed

GromNaN deleted the type-from-php-variable branch October 24, 2025 14:19


		This pattern can be adapted for any custom class—just implement the conversion logic for your value object.

		.. \|FQCN\| raw:: html <abbr title="Fully-Qualified Class Name">FQCN</abbr>

		if ($variable instanceof ObjectId) {
		return self::getType(self::ID);


		<field field-name="field" type="date_with_timezone" />

		Custom Type Example: Mapping a UUID Class

Uh oh!

Uh oh!

Improve detection of the type from a PHP variable #2814

Improve detection of the type from a PHP variable #2814

Uh oh!

Conversation

GromNaN commented Sep 1, 2025

Summary

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GromNaN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!